The new field for formatter_out_functions will break any code that creates such a record "from scratch" (i.e. without overriding some fields of an existing record). At least for the future, it would be nice to expose a builder function so that the record can be further extended without breaking client code (using optional arguments for new fields).

Alain, yes indeed it is conceivable that someone does build a
formatter_out_functions record from scratch, but this is really not probable,
since the easy way to modify the record is indeed to use a with clause.
In any case, modifying such an hypothetical piece of code will be easy using
a with clause and the resulting code will be even clearer.

Also note that this record has not been extended for decades and the new
field is the last semantic action that the pretty-printer indeed performs
that was not part of the output function record.

I think the best way to achieve a future proof API would be to abstract the
record and only provide functions to modify an existing record. But this
would break even more existing code.

The new field for formatter_out_functions will break any code that creates such a record "from scratch" (i.e. without overriding some fields of an existing record). At least for the future, it would be nice to expose a builder function so that the record can be further extended without breaking client code (using optional arguments for new fields).

---

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#615 (comment)

I've checked LexiFi's code base. We have three occurrences where we build a custom formatter_out_functions record. Two of them list explicitly all the fields; one uses the record override form. Actually, this last form would be the trickiest one, since it should semantically be adapted to override out_indent as well but the compiler would not complain. This could lead to subtle pretty-printing bugs. The other two places are indeed trivial to fix, but this makes it impossible to have code compatible with previous versions.

We are generally very conservative with breaking changes in the stdlib (more than I usually wish). It would be worth checking on public OPAM packages to find out the extent of the breakage.

Can you elaborate on how this change is related to the title of the proposal? Without looking at the details, it seems to me that making a distinction between ident and spaces is not strictly related to that, or is it?

I think the best way to achieve a future proof API would be to abstract the record and only provide functions to modify an existing record. But this would break even more existing code.

Well, it seems almost any code that manipulates formatter_out_function would need to be adapted anyway; so perhaps it is best breaking compatibility frankly, if only to prevent silent bugs (as described above).

One could also provide a builder function (easy to extend with optional arguments) and mark the concrete record as being deprecated, without actually dropping it.

@alainfrisch I have run an over-approximated analysis over all opam packages for 4.03.
Only 3 packages reference the record fields and only 1 constructs it directly (utop). The very same construction appears in OCaml's testsuite and is fixed in #595.

See the details in log_analysis.txt

Update: this message is outdated (see Octachron's comment below).

@pierreweis, @rbonichon: this change lacks a Changes entry, so it is not advertised to users of the release -- and they don't know where to look for a reference to this PR if they want to understand the design rationale, etc.

Would it be possible for you to submit a Changes entry (as a new PR), following the CONTRIBUTING.md#changelog guidelines?

At least the following people should be mentioned in the "review by" credits: Gabriel Radanne and Florian Angeletti (that is, Drup and octachron, the first did a review of the patch and the second fixed since-tags and documentation issues after the fact).

Given that we are planning to release by the end of the month, it would be very nice if you could do this (small thing) next week. If you think you will not have the time, please let us know as soon as possible, and @Octachron or myself will write something.

@gasche , in fact I had already added a Change entry for the symbolic printer feature
https://github.com/ocaml/ocaml/blob/trunk/Changes#L135 ,
which even contained a short mention of the formatter_out_functions change,
but I had somehow missed the link with #595 and Drup's review.
Sorry for the resulting confusion.

Nevertheless, it would be probably clearer to split the change entry in two: one entry for the symbolic printer and another one fot the out_indent field.

One of the packages that breaks because of the new record field is notty : see pqwy/notty#17. I'm linking the issue here for reference.

It would not break if not for this.

Having

let display_indent state = state.pp_out_spaces

would enable code to ignore the new feature while retaining the old behavior, which is presently impossible.

That is, the new operation should be defined in terms of the old operation it is partially replacing.

@pqwy This is exactly the proposal of #1382

There are also drawbacks to keeping the old behavior as you suggest, as I explain in the discussion of said PR. Basically indenting is a separate concept from outputting spaces (think of the tab vs space debate for indentation).